Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More raise less fail #1008

Merged
merged 3 commits into from
Jul 25, 2024
Merged

More raise less fail #1008

merged 3 commits into from
Jul 25, 2024

Conversation

raphael-proust
Copy link
Collaborator

raise and reraise are "better" than fail for backtracing and such

@smorimoto smorimoto force-pushed the more-raise-less-fail branch from 7113b11 to 5eee5f0 Compare April 29, 2024 13:10
@smorimoto smorimoto force-pushed the more-raise-less-fail branch from 5eee5f0 to d0b0e8b Compare June 20, 2024 06:29
@MisterDA
Copy link
Contributor

Is that documented in the manual/docs? Should code using Lwt switch to raise/reraise?

@smorimoto smorimoto merged commit 266f173 into master Jul 25, 2024
54 of 55 checks passed
@smorimoto smorimoto deleted the more-raise-less-fail branch July 25, 2024 15:11
@raphael-proust
Copy link
Collaborator Author

Is that documented in the manual/docs? Should code using Lwt switch to raise/reraise?

reraise is documented in the doc https://github.com/ocsigen/lwt/blob/master/src/core/lwt.mli#L581 / https://ocaml.org/p/lwt/latest/doc/Lwt/index.html#val-reraise

and it is recommended to use in the documentation of catch and try_bind.

The use of raise (instead of fail) is recommended in the documentation of fail and has been for quite some time now: https://github.com/ocsigen/lwt/blob/master/src/core/lwt.mli#L453 / https://ocaml.org/p/lwt/latest/doc/Lwt/index.html#val-fail

Use Lwt.fail only when you specifically want to create a rejected promise, to pass to another function, or store in a data structure.


Note that it's better for users to use raise+reraise, but ultimately, even if they use fail this PR does not affect them negatively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants